Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AJ-1094: Initial Project Setup and Hello World CLI #1

Merged
merged 46 commits into from
Jul 20, 2023
Merged

Conversation

snf2ye
Copy link
Collaborator

@snf2ye snf2ye commented Jun 21, 2023

https://broadworkbench.atlassian.net/browse/AJ-1094


Background

This pull request provides the base repo structure for the java-pfb library. Our goal is to reproduce the pyPFB library, but in the format of a Java client. We will use the generated Java client in WDS to parse incoming files in the PFB format. We will also create command line tools so that java-pfb can be used in a similar manner to pyPFB.

General Outline of modules included in repo:

  • library module: Contains code to perform operations on PFB files
  • cli module: contains CLI-related code and will reference the pfb client

Testing out the changes

First, run "jar" gradle task in cli project.

./gradlew :cli:jar

Then, you can use the CLI with the following command:

java -cp "cli/build/libs/java-pfb-cli.jar" bio.terra.pfb.JavaPfbCommand <command>

Available Commands:

  • hello
  • --version
  • --help

Review Requests

  • Feedback on Repo/Direcotry structure for a java client and CLI
  • Feedback on naming the modules. I've defaulted to "javapfb"
  • I have left a least a little bit of placeholder/boilerplate code leftover from the java template project. I've gone through and deleted a lot of code, but please let me know if you see any code left that we will definitely not use that I missed.

Github Actions

I don't expect to fully work out all of the GHAs before merging this, like the publish action. This will be handled in a follow-on ticket. Some questions to consider in the meantime:

  • Do we want to alert our channel on every PR run failure?
  • Do we need integration tests? [I don't think so. Services that use the client will have integration tests. We will have unit tests.]
  • Publish action: I think we just need to bump the tag and publish the client to artifactory. I don't think we need to report the version to sherlock. <-- To be handled in next PR [Also, take a look at the generator.gradle git properties and set up a --version command]

Remaining TODOs

  • Merge terraform-ap-deployments PR in order to get BROADBOT_TOKEN and SONAR_TOKEN enabled in repo [Some GHAs will fail until these secrets are populated]
  • Fix remaining failing GHAs (Still failing after merging PR)
  • [NOTE: This is not yet resolved, but I don't think it should hold up the PR] Get sonar scan working for both CLI and library projects (right now only running on library). Looking into using the monorepo setup on sonar and working with Tom to get this set up.
  • Build jar with gradle application plugin rather than the custom fatjar command I currently have set up (update: still needed the custom task, but could keep the name of the task "jar")
  • Fix library main method so it's not empty [Example: https://github.com/DataBiosphere/terra-data-catalog/blob/main/common/src/main/java/bio/terra/catalog/CommonApp.java#L33]
  • Add test coverage so that we have 100% test coverage for our very first PR 😄
  • Adjust java package name for main CLI file
  • Use new setup for picocli code (fix codesmell)

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

add workflows

Revert "rename to java pfb"

This reverts commit 166b8c9b6910920a20de9156a4bbb304652fdd84.

rename to java pfb
@snf2ye snf2ye force-pushed the sh-initial-setup branch 4 times, most recently from 47919e4 to 97c0103 Compare June 22, 2023 14:44
Remove template docs

Remove datasource from App
See questions in PR descriptions
@snf2ye snf2ye force-pushed the sh-initial-setup branch 2 times, most recently from 50a7cb3 to 843eef0 Compare June 22, 2023 18:27
add help command
@snf2ye snf2ye marked this pull request as ready for review June 23, 2023 20:04
.github/workflows/publish.yml Outdated Show resolved Hide resolved
settings.gradle Outdated Show resolved Hide resolved
.github/workflows/trivy.yml Outdated Show resolved Hide resolved
LICENSE Outdated Show resolved Hide resolved
buildSrc/build.gradle Outdated Show resolved Hide resolved
lib/publishing.gradle Outdated Show resolved Hide resolved
lib/src/main/java/bio/terra/javapfb/Library.java Outdated Show resolved Hide resolved
lib/src/main/java/bio/terra/javapfb/Library.java Outdated Show resolved Hide resolved
lib/src/main/java/bio/terra/javapfb/Library.java Outdated Show resolved Hide resolved
lib/src/main/java/bio/terra/javapfb/Library.java Outdated Show resolved Hide resolved
snf2ye and others added 4 commits July 10, 2023 12:25
Update README.md
Copy link
Contributor

@calypsomatic calypsomatic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks great! Thank you!

Copy link
Collaborator

@davidangb davidangb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good! A few comments inline, all of which can be deferred for future PRs.

uses: broadinstitute/workflow-dispatch@v1
with:
workflow: Tag
token: ${{ secrets.BROADBOT_TOKEN }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we will want to tag on every push to main - but I'm totally fine leaving this as-is and addressing in the future, once we have a better idea of what we DO want

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok! I'm inclined to tag on every push to main, but agreed - let's leave as-is and we'll address it in the future once we've had a chance to discuss!

Comment on lines +17 to +19
implementation 'com.diffplug.spotless:spotless-plugin-gradle:6.11.0'
implementation 'com.srcclr.gradle:com.srcclr.gradle.gradle.plugin:3.1.12'
implementation 'org.sonarqube:org.sonarqube.gradle.plugin:4.2.1.3168'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will these be included in, or required by, the final jar we publish to artifactory? I haven't tried it myself to see what the results are. If these ARE included or required, can that be prevented, potentially by choosing a configuration other than implementation? We want the published jar to be as small and as dependency-free as possible; and these are only used by build tasks, not at runtime, correct?

I'm fine fixing this in a follow-on PR if you want to defer this work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point, I will make sure this is covered with AJ-1095

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file (buildSrc/build.gradle) is different than other gradle project configuration files. It's (normally) only used to configure plugins, and its "dependencies" block only affects the plugins loaded by gradle, not compile or runtime dependencies used when running gradle tasks.

So these dependencies have no effect on the jar published to artifactory. They're here so when a plugin is used elsewhere in the project you don't need to specify the version. It provides a way to make sure that the same plugin versions are used in all subprojects.

With that in mind, the repositories block should only include those that are needed to load plugins. In my project I use:

repositories {
    maven {
        url 'https://broadinstitute.jfrog.io/artifactory/plugins-snapshot'
    }
    gradlePluginPortal()
}

The artifactory configuration is needed for the terra-test-runner plugin which is locally published. If you aren't using that plugin then gradlePluginPortal() should be sufficient.

Also I'd remove the test block below. I don't think it has any effect, and it should go in one of the plugin subprojects instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pshapiro4broad thanks for this context, I think I understand the buildSrc setup a little better now.

gradle/wrapper/gradle-wrapper.properties Outdated Show resolved Hide resolved
Copy link

@TomConner TomConner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how this is set up as two SonarCloud projects.

@snf2ye snf2ye changed the title AJ-1094: Java template project and Hello World CLI AJ-1094: Initial Project Setup and Hello World CLI Jul 20, 2023
Copy link
Member

@pshapiro4broad pshapiro4broad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few more comments, looks good to me overall!

name = "pfb",
mixinStandardHelpOptions = true,
description = "A java implementation of pyPFB",
version = "java-pfb 0.1.0")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you want to do it now, but I think this should use picocli's dynamic version support and use the data generated by the gradle-git-properties plugin. That would help confirm that the git-properties plugin is working as expected.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok! I started taking a look at this, but I think this PR is at a good stopping point to be merged. So, I will take a look at this with my next chunk of publishing work!


sonar {
properties {
property "sonar.projectKey", "DataBiosphere_java-pfb-cli"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also wanted to set up sonar scanning for a project that has multiple gradle subprojects but hadn't looked into it yet. i think there's a way to combine scans (and code coverage data) across different subprojects, did you look into doing it this way before running the scans as separate projects? How does this work on PRs? Does it show the result of both scans?

Copy link
Collaborator Author

@snf2ye snf2ye Jul 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question -- I was hoping to use the monorepo setup that's available for sonarcloud that I think should do what you're talking about. But, it's really hard to fiddle with settings because engineers don't have access to the admin settings on sonarcloud. Tom flipped the monorepo switch on the project, but as far as I could tell, nothing changed. The two projects still went into two separate sonarcloud projects.

This technically works -- I have two steps in GHA that runs against the two projects. Sonarcloud comments twice in the PR, once for each project (unfortunately, the comments don't distinguish between the two projects).

I think this is good enough for now, but I would definitely be interested in looking at the monorepo setup more -- this would require more time from infosec or more permissions in sonarcloud.

cli/build.gradle Outdated Show resolved Hide resolved
Revert "upgrade gradle to 8.2.1"

This reverts commit 525bbd8.

Revert "Revert "upgrade gradle to 8.2.1""

This reverts commit 200cced.
@snf2ye
Copy link
Collaborator Author

snf2ye commented Jul 20, 2023

Note: Waiting on https://github.com/broadinstitute/dsp-appsec-sourceclear-github-actions/pull/103 to be merged before the results of the source clear scan is uploaded.

@sonarcloud
Copy link

sonarcloud bot commented Jul 20, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@sonarcloud
Copy link

sonarcloud bot commented Jul 20, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

62.5% 62.5% Coverage
0.0% 0.0% Duplication

@snf2ye
Copy link
Collaborator Author

snf2ye commented Jul 20, 2023

Note: Waiting on broadinstitute/dsp-appsec-sourceclear-github-actions#103 to be merged before the results of the source clear scan is uploaded.

dsp-appsec-sourceclear-github-actions PR cannot be merged until there is code in the java-pfb repo, so I will confirm that this is setup works after this java-pfb PR is merged.

@snf2ye snf2ye merged commit a49d68c into main Jul 20, 2023
7 checks passed
@snf2ye snf2ye deleted the sh-initial-setup branch July 20, 2023 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants